Match recorded violations in strict packs (Part A) - #43

Blocking: pks update erases the entries this now depends on.

write_violations_to_disk drops every strict violation when regenerating todo files:

// src/packs/package_todo.rs:144
if violation.identifier.strict {
    continue;
}

That line is pre-existing and untouched here, but this filter is what starts depending on those todo entries. On main the asymmetry was invisible, because check failed either way. Now, on tests/fixtures/uses_strict_mode, with no source change in between:

$ pks check
No violations detected!                      # exit 0, working as intended

$ pks update
2 strict mode violation(s) detected. These violations must be fixed for `check` to succeed.
Successfully updated package_todo.yml files!  # exit 0
# packs/foo/package_todo.yml is now DELETED. Both of that pack's recorded
# violations are strict, so nothing is written for foo and the None branch
# hits delete_package_todo_from_disk.

$ pks check
2 violation(s) detected: ...
packs/foo cannot have privacy violations on packs/bar because strict mode is enabled ...
                                             # exit 1

A routine pks update un-grandfathers every recorded violation in a strict pack and turns a green build red. Your real-app numbers hold until someone runs update.

I'd call this blocking rather than a pre-existing quirk to port later, because packwerk does the opposite here:

# lib/packwerk/offense_collection.rb#add_offense
if strict_mode_violation?(offense)
  add_to_package_todo(offense) if already_listed
  strict_mode_violations << offense
else
  add_to_package_todo(offense)
end

An unlisted strict violation never gets added, so you can't silence strict mode by running update-todo. An already-listed one gets re-added, and that re-add is what keeps the entry in the file, since PackageTodo#dump writes new_entries wholesale. packwerk protects the state its own check tolerance reads. pks treats both cases the same.

Suggested fix: in write_violations_to_disk, drop only the unlisted strict violations. The recorded set is already at configuration.pack_set.all_violations, the same source CheckAllBuilder uses, and the comparison needs recorded_key(), so Part A comes first.

This doesn't require changing tests/update_test.rs:199-225. That test runs against contains_strict_violations, which ships no package_todo.yml and gets remove_file'd first, so its violation is unlisted, and "todo should not be created for strict violations" is what packwerk does in that case. The already-listed case has no test, which is how this stayed hidden.

Design note, non-blocking, and fine to defer to a follow-up.

Consider moving strict off ViolationIdentifier and onto Violation instead of normalizing at comparison time. Your comment here already says why: strict describes how a violation should be treated, not which violation it is. The doc comment just below at checker.rs:55-64 sets the same rule for source_location, that the identifier defines sameness for comparison against package_todo.yml, "which doesn't store line/column." strict isn't stored there either.

The change is mechanical. Every reader of .identifier.strict (json.rs:56,90; csv.rs:12,53; package_todo.rs:144) already has a full &Violation, and build_strict_violation_message never reads the field. Constructors are pack.rs:195, which is where #41 starts and which then stops having to invent strict: false, plus pack_checker.rs:180 and four test constructors. You'd get all three comparison sites back to plain contains(&v.identifier), #41 becomes impossible to express instead of something a future call site has to remember to guard, and the extra allocations go away.

One alternative to skip: excluding strict from a manual PartialEq/Hash. Violation's derived Eq/Hash delegate to the identifier, and get_all_violations dedupes into a HashSet<Violation>, so making strict: true equal strict: false lets an insert keep the wrong flag, which build_strict_mode_violations then filters on.

recorded_key() is correct as written. This is about where the field lives, not about a bug.

Nit: this call does nothing. todo_violation_identifier comes from pack_set.all_violations, which is always built with strict: false (pack.rs:195), so recorded_key() clones 4 Strings per recorded violation and changes nothing. I reverted just this call and the whole suite stays green.

The found-side .map(|v| v.identifier.recorded_key()) above is the one doing the work. Either drop this one or add a comment saying recorded identifiers arrive already normalized, so a future reader doesn't assume it matters.

Minor: this moves from HashSet<&ViolationIdentifier> to an owned HashSet<ViolationIdentifier>, so it now clones 4 Strings per found violation rather than copying a pointer. Small next to parsing 15.6k files, so fine to leave.

If you want the cheaper version, a borrowed key tuple that excludes strict avoids the allocations entirely. Moving strict onto Violation (see my note on recorded_key) would also let this go back to borrowing.

Test gap worth pinning: a strict pack with some recorded and some unrecorded violations in the same run.

I checked and the behavior is right. Adding a second, unrecorded reference alongside the recorded ::Bar in this fixture reports only the new one. That's the case that makes strict mode adoptable, and nothing in the suite covers it today, so a regression here would be silent.

Also worth a check -> update -> check test on this fixture, asserting the second check is still clean. That's the round trip that currently breaks (see my comment on build_strict_mode_violations).

No coverage lost here, just flagging why for the record: the removed line is byte-identical to the one kept below it, so this drops a duplicate assertion.

The duplication was pointing at something real, though. Unrecorded strict violations get reported twice, since build_reportable_violations doesn't filter on .strict and the formatters concatenate both sets, which is why -o csv emits the same row twice on this fixture. It reproduces on main, so it's pre-existing and not yours to fix here.